feat: execute droptable and reopen tasks in batch#421
Conversation
WalkthroughAdds a new KvOptions field Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant EloqStore
participant Scheduler as ScheduleState
participant Worker as ExecAsync/PartitionHandler
Client->>EloqStore: Send global operation (archive/reopen/drop)
EloqStore->>Scheduler: Create ScheduleState (targets[], max_inflight)
Scheduler->>Worker: Launch up to max_inflight subrequests
Worker-->>Scheduler: Subrequest result (ok / error)
Scheduler-->>Worker: Launch next subrequest (until all started)
Scheduler->>EloqStore: All subrequests completed (aggregate result)
EloqStore-->>Client: Respond with final status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/eloq_store.cpp (1)
933-938: Make the global scheduler bounded end-to-end.Each handler now limits concurrent dispatch, but it still allocates one subrequest object per partition before the first send. That keeps heap growth proportional to partition count and repeats the same scheduler skeleton three times. A small shared helper that keeps only a bounded pool of live subrequests and creates them lazily would remove the duplication and make
max_global_request_batchcover memory as well as concurrency.Also applies to: 940-1008, 1176-1187, 1192-1260, 1398-1407, 1409-1496
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/eloq_store.cpp` around lines 933 - 938, The current handlers eagerly allocate one subrequest per partition (e.g., creating TruncateRequest instances and pushing into req->truncate_reqs_) which allows heap growth proportional to partition count; refactor by introducing a small shared helper (e.g., a BoundedSubrequestPool or make_lazy_subrequest_generator) that maintains at most max_global_request_batch live subrequests, creates TruncateRequest (and the other subrequest types used in the other handlers) lazily when about to dispatch, and recycles/completes them when a dispatched subrequest finishes; replace the per-partition eager loops that push into truncate_reqs_ with iteration that obtains subrequests from this pool and sends them, and reuse the same helper across the three similar handler sites (the ranges called out) so memory is bounded by max_global_request_batch as well as concurrency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/kv_options.cpp`:
- Around line 178-182: When loading max_global_request_batch in KvOptions (the
reader.HasValue/GetUnsigned block for sec_run and max_global_request_batch),
ensure a zero value is not accepted: after reading the unsigned value, normalize
0 to 1 (or reject by returning an error); additionally add a defensive check in
ValidateOptions() to assert max_global_request_batch > 0 and fail validation if
not. Update the reader.GetUnsigned usage for max_global_request_batch and the
ValidateOptions() logic in KvOptions to reference the same semantic (must be >
0).
---
Nitpick comments:
In `@src/eloq_store.cpp`:
- Around line 933-938: The current handlers eagerly allocate one subrequest per
partition (e.g., creating TruncateRequest instances and pushing into
req->truncate_reqs_) which allows heap growth proportional to partition count;
refactor by introducing a small shared helper (e.g., a BoundedSubrequestPool or
make_lazy_subrequest_generator) that maintains at most max_global_request_batch
live subrequests, creates TruncateRequest (and the other subrequest types used
in the other handlers) lazily when about to dispatch, and recycles/completes
them when a dispatched subrequest finishes; replace the per-partition eager
loops that push into truncate_reqs_ with iteration that obtains subrequests from
this pool and sends them, and reuse the same helper across the three similar
handler sites (the ranges called out) so memory is bounded by
max_global_request_batch as well as concurrency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 63e013d1-96b6-4731-94f5-cf2bba06162e
📒 Files selected for processing (3)
include/kv_options.hsrc/eloq_store.cppsrc/kv_options.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/eloq_store.cpp (1)
945-1013: LGTM - Well-structured batch scheduling pattern.The
DropTableScheduleStateimplementation is sound:
- Proper use of
shared_from_this()for safe callback lifetime management- Correct memory ordering:
acq_relonpending_ensures visibility offirst_error_writes- Graceful failure handling with callback clearing before
SetDoneNote that
DropTableScheduleState,ArchiveScheduleState, andReopenScheduleStateare nearly identical. Consider consolidating into a template or helper to reduce duplication in a follow-up PR.
,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/eloq_store.cpp` around lines 945 - 1013, The three nearly identical scheduler structs (DropTableScheduleState, ArchiveScheduleState, ReopenScheduleState) should be consolidated into a single templated or generic helper to remove duplication: extract the common members (store, req, total, next_index) and methods (HandleTruncateResult, OnTruncateDone, ScheduleNext) into a template class or a policy-based helper that accepts the specific request type (e.g., TruncateRequest container/accessor) and callback behavior; update the call sites to instantiate the template with the appropriate request type and preserve current semantics (shared_from_this usage, memory orderings, error handling) so functionality and lifetime management remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/eloq_store.cpp`:
- Around line 945-1013: The three nearly identical scheduler structs
(DropTableScheduleState, ArchiveScheduleState, ReopenScheduleState) should be
consolidated into a single templated or generic helper to remove duplication:
extract the common members (store, req, total, next_index) and methods
(HandleTruncateResult, OnTruncateDone, ScheduleNext) into a template class or a
policy-based helper that accepts the specific request type (e.g.,
TruncateRequest container/accessor) and callback behavior; update the call sites
to instantiate the template with the appropriate request type and preserve
current semantics (shared_from_this usage, memory orderings, error handling) so
functionality and lifetime management remain unchanged.
Here are some reminders before you submit the pull request
fixes eloqdb/eloqstore#issue_idctest --test-dir build/tests/Summary by CodeRabbit
New Features
Refactor